Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xplat-intl: default to non intl compare for localeCompare #4625

Merged
merged 1 commit into from
Jan 31, 2018

Conversation

obastemur
Copy link
Collaborator

Fixes #4623

Also enabling a test case that only tests the interface.

if (compareResult == 0 && size1 != size2)
{
compareResult = size1 > size2 ? 1 : -1;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to add 2 to compareResult because CompareStringEx returns 0 on error, 1 for less, 2 for equal, and 3 for greater. The rest of the code below assumes this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kept the impl. separate and added a comment to make the intention clear -> (once ICU interface supports this, keep the implementation below for non-icu!)

@jackhorton
Copy link
Contributor

Also of note, this will be fixed even more once intl-icu merges back into master.

Copy link
Contributor

@dilijev dilijev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo Jack's comment.

@obastemur
Copy link
Collaborator Author

@jackhorton @dilijev Thanks for the review!

@chakrabot chakrabot merged commit fa6274f into chakra-core:release/1.8 Jan 31, 2018
chakrabot pushed a commit that referenced this pull request Jan 31, 2018
…localeCompare

Merge pull request #4625 from obastemur:xplat_intl_local_comp

Fixes #4623

Also enabling a test case that only tests the interface.
chakrabot pushed a commit that referenced this pull request Jan 31, 2018
…mpare for localeCompare

Merge pull request #4625 from obastemur:xplat_intl_local_comp

Fixes #4623

Also enabling a test case that only tests the interface.
chakrabot pushed a commit that referenced this pull request Feb 1, 2018
…o non intl compare for localeCompare

Merge pull request #4625 from obastemur:xplat_intl_local_comp

Fixes #4623

Also enabling a test case that only tests the interface.
@rhuanjl
Copy link
Collaborator

rhuanjl commented Feb 1, 2018

@obastemur this doesn't seem to be working on xplat in 1.9 and master - I think it has broken CI (though it works on 1.8)

@obastemur
Copy link
Collaborator Author

The reason is the test baseline on release/1.9 doesn't match with spec. While spec says the result should be neg, zero, or positive, the baseline checks for a particular integer. Sending the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants